Skip to content

Conversation

jpadilla
Copy link
Contributor

If NUM_PROXIES setting is set to None, HTTP_X_FORWARDED_FOR might be used as is, which
might contain spaces and cause errors on cache backends like memcached. Ref #2400

If NUM_PROXIES setting is set to None,
HTTP_X_FORWARDED_FOR might be used as is, which
might contain spaces and cause errors on
cache backends like memcached.
ident = request.META.get('REMOTE_ADDR')
else:
ident = ''.join(ident.split())

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why those were removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xordoquy they weren't being used at all.

@jpadilla
Copy link
Contributor Author

Note: This happens in both v2 and v3.

@lovelydinosaur
Copy link
Collaborator

Looks okay to me - anyone else want to second it?

@@ -35,7 +35,7 @@ def get_ident(self, request):
client_addr = addrs[-min(num_proxies, len(xff))]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in #2400 (comment), this should be len(addrs) instead of len(xff).

@jpadilla jpadilla added this to the 3.0.4 Release milestone Jan 12, 2015
@jpadilla
Copy link
Contributor Author

This should be good to go. Thanks for the reviews.

lovelydinosaur added a commit that referenced this pull request Jan 12, 2015
Fix ident format when using HTTP_X_FORWARDED_FOR
@lovelydinosaur lovelydinosaur merged commit 69fea56 into encode:master Jan 12, 2015
@jpadilla
Copy link
Contributor Author

@tomchristie not sure if it's worth fixing this in 2.4 as well. Seems to be workable around easily. Let's see if it comes up again.

@xordoquy
Copy link
Contributor

The general idea was to drop the 2.4 support since our time is limited.

@lovelydinosaur
Copy link
Collaborator

not sure if it's worth fixing this in 2.4 as well

I'd have no great argument if somebody wanted to spend their time on that, but I don't think it's a good effort/reward myself, so unless someone wants to get really proactive with making a further 2.4.x release happen then it's a no from me.

See https://groups.google.com/forum/#!searchin/django-rest-framework/2.4/django-rest-framework/NHcDGGPaqNw/Y64_7ipJcX0J

@jpadilla
Copy link
Contributor Author

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants